Allow meshes to be renamed on load - #452
Conversation
| subroutine load_global_mesh_multiple( input_mesh_file, & | ||
| mesh_names ) | ||
| mesh_names, & | ||
| rename_to ) |
There was a problem hiding this comment.
Not sure what you mean, the rename_to argument is an optional argument
There was a problem hiding this comment.
Ah, you mean Doxygen comments
| !> from the <input_mesh_file>. | ||
| subroutine load_global_mesh_single( input_mesh_file, & | ||
| mesh_name ) | ||
| mesh_name, rename_to ) |
| stencil_depths, & | ||
| generate_inner_halos, & | ||
| partitioner_ptr, & | ||
| mapping_factors, & |
|
|
||
|
|
||
|
|
||
| subroutine create_local_mesh_maps_from_object( source_local_mesh ) |
|
|
||
| return | ||
| end subroutine create_local_mesh_maps | ||
| end subroutine create_local_mesh_maps_from_file |
There was a problem hiding this comment.
There seems to be quite a bit of code duplication between create_local_mesh_maps_from_file and create_local_mesh_maps_from_object and so I wonder whether these could/should be combined in some way.
| w0_domain_fs => function_space_collection%get_fs(mesh, 0, 0, W0) | ||
| w3_domain_fs => function_space_collection%get_fs(mesh, 0, 0, W3) | ||
|
|
||
| call init_xios_axis( "vert_axis_full_levels", w0_domain_fs ) |
There was a problem hiding this comment.
Please can you explain why you've changed the call to init_xios_axis to domain_fs from mesh, W0. Is it necessary for this mesh renaming PR or just other tidying up?
| end function is_local | ||
|
|
||
| !------------------------------------------------------------------------------ | ||
| !> @brief Returns original mesh name as referenced in the orignal source file. |
There was a problem hiding this comment.
Typo orignal to original
|
|
||
|
|
||
| !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! | ||
| !> @brief Returns original mesh name as referenced in the orignal source file. |
| integer(i_def) :: num_xprocs, num_yprocs | ||
|
|
||
| call log_event("Using custom decomposition", LOG_LEVEL_INFO) | ||
| call log_event("using custom decomposition", log_level_debug) |
There was a problem hiding this comment.
Given all the problems the C896 etc have been having with custom decompositions I suggest it would be wise to keep this as LOG_LEVEL_INFO for now. Its not necessary for renaming meshes - just a tidyup.
| integer(i_def) :: start_xprocs, start_width, i | ||
| logical ::found_factors | ||
|
|
||
| call log_event("Using auto_nonuniform decomposition", LOG_LEVEL_INFO) |
There was a problem hiding this comment.
Again, please can we keep this as LOG_LEVEL_INFO for now.
| " must be the product of xprocs ", self%num_xprocs, & | ||
| " and yprocs ", self%num_yprocs | ||
| call log_event( log_scratch_space, LOG_LEVEL_ERROR ) | ||
| call log_event( log_scratch_space, log_level_error ) |
There was a problem hiding this comment.
Most of the code base has been using upper-case for LOG_LEVEL for quite some time. So I'm wondering if there is a centralised push to change these to lower-case? Or is this just personal preference? I would suggest that they are removed from this ticket (as its just adding to the length for the reviewer!) and that IF there is centralised agreement to change these that a separate ticket is created to change all of the log_level messages.
|
|
||
| integer(i_def) :: num_xprocs, num_yprocs | ||
|
|
||
| call log_event("Using row decomposition", LOG_LEVEL_INFO) |
There was a problem hiding this comment.
Please leave as LOG_LEVEL_INFO.
|
|
||
| integer(i_def) :: num_xprocs, num_yprocs | ||
|
|
||
| call log_event("Using column decomposiiton", LOG_LEVEL_INFO) |
There was a problem hiding this comment.
Please leave as LOG_LEVEL_INFO
| integer(i_def) :: num_xprocs | ||
|
|
||
| call log_event("Using guided_nonuniform decomposition", LOG_LEVEL_INFO) | ||
| call log_event("Using guided_nonuniform decomposition", log_level_debug) |
There was a problem hiding this comment.
Please leave as LOG_LEVEL_INFO
| call log_event( log_scratch_space, lOG_LEVEL_INFO ) | ||
| write(log_scratch_space,"(a,i0,a,i0)") & | ||
| "Number of cells in partition ", num_x, " X ", num_y | ||
| call log_event( log_scratch_space, lOG_LEVEL_DEBUG ) |
There was a problem hiding this comment.
Please leave as LOG_LEVEL_INFO
| logical(l_def) :: test_dofmap | ||
|
|
||
| type(local_mesh_type) :: unit_test_local_mesh | ||
| integer(i_def) :: mesh_id |
There was a problem hiding this comment.
There seems to be quite a lot of changes changing mesh_id to mesh. Are these necessary for this PR? Or unrelated tidying up. If the latter, it should really go on a separate PR.
| test_str2 = global_mesh%get_origin_name() | ||
| @assertTrue(trim(test_str2) == trim(mesh_name)) | ||
|
|
||
| test_str2 = global_mesh%get_mesh_name() |
There was a problem hiding this comment.
I think a different variable e.g. test_str3 should be used here
|
Sci/Tech review: Thanks for making these changes to enable using meshes with the same name in lfric2lfric. I am generally happy with the implementation with only a couple of minor comments. But please could I encourage the developer to move any changes that are not essential to this work to a separate PR - it will make it a lot easier for reviewers to give a proper assessment of the work. |
PR Summary
Sci/Tech Reviewer: cjohnson-pi
Code Reviewer: Matthew Hambley (@MatthewHambley)
Access to meshes in input files produced from the mesh generators should contain unique mesh names by design. The mesh names are used as a more user-friendly means to reference meshes in downstream applications. This however becomes an issue when applications such as lfric2lfric are required to read in multiple files produced for other applications. Applications which generate their input files may have used mesh(es) with identical names (e.g. dynamics, multigrid_l1, etc) in their individual tasks which produce output.
Adopting a practice of naming a mesh after the process/scheme that uses it, rather than a name that describes the mesh is not ideal. However, this practice has come about as a result of XIOS iodef.xml files which reference the specific mesh name, having the same mesh names allow for a template iodef.xml to be used. The alternative would significantly increase the number of iodef.xml files (with only a mesh name change between them). Having a constant mesh name, thus results in meshes from separate files having the same name.
This poses a problem for applications like lfric2lfric, where it needs to read different meshes from one or more files, were the meshes might have the same name in each of the files.
The simple solution here is to allow an application to read in the meshes (which have the same name) from individual files as separate instances and allow the application (lfric2lfric) to rename when the instance are created. This allows a method for an application to not violate the unique naming of objects restrictions in the mesh collections. This neatly allows lfric2lfric to use the meshes it requires (with the names they wish) without impacting the upstream applications that produced the input files.
Implementation
Allowing the meshes to be renamed before storing them in the collections is relatively straight forward. An optional argument on the method on the mesh constructors (global/local) allow the mesh objects to be renamed on creation before storing them in the collection. After that, the application should only worked with the "stored mesh names".
Complications
Mesh inter-grid maps: These where designed to be valid at the point of mesh generation, i.e. applications should only use maps between meshes sourced from the same file. In this case, the meshes are being renamed and potentially from separate files. The maps themselves are logged from mesh to mesh via their ids, which remain unique, so that shouldn't be an issue. The complication comes when identifying which maps from which files should be assigned to which mesh during setup.
The solution here is to log at the point of creation the original mesh name and file that the mesh came from. This means that during the intergrid map setup, maps are only read, created and attached if the meshes (using their original names) came from the same file. After the maps are read and attached, the rest of the code should proceed as normal.
Chi transforms (Out-of-Scope see Refactor code implementing initial chi transform with respect to North Pole and Schmit stretching #460): This does a lot of unnecessary things, however changes to make it more sensible change kgos. So it should be done as separate PR.
Mapping factors: Implementation of mapping factors is too low in the code, in addition it computes for every mesh in the mesh collection which is overkill. It should only need to do this for meshes that are required to be aligned. The routine was overloaded so that a mapping factor could be computed between a global_mesh and a single reference global mesh rather than the whole collection.
Potential downstream issues
Linked PRs
Code Quality Checklist
Testing
Test Suite Results - lfric_core - lfric2lfric_in_and_out/run1
Suite Information
Task Information
✅ succeeded tasks - 433
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review